Skip to content

Conversation

@nwidynski
Copy link
Contributor

@nwidynski nwidynski commented Aug 9, 2025

A Scrollport coincides with the padding box, which should not include border offset. We were calculating on offsetWidth & bounding boxes instead of clientWidth and for some reason shifted the item rect by scroll offset??

With this change, the DOM layout is now consistent with our virtualized ones 👍

✅ Pull Request Checklist:

  • Included link to corresponding React Spectrum GitHub Issue.
  • Added/updated unit tests and storybook for this change (for new code or code which already has tests).
  • Filled out test instructions.
  • Updated documentation (if it already exists for this component).
  • Looked at the Accessibility Practices for this feature - Aria Practices

📝 Test Instructions:

🧢 Your Project:

@nwidynski nwidynski changed the title fix: subtract border width from visible rect Fix: DOMLayoutDelegate miscalculates visible rect Aug 9, 2025
@nwidynski nwidynski changed the title Fix: DOMLayoutDelegate miscalculates visible rect Fix: DOMLayoutDelegate miscalculates item & visible rects Aug 10, 2025
@snowystinger
Copy link
Member

Hey, we were testing this today and ran into some issues. I'm not entirely sure if this is what we should be testing, but we went to https://reactspectrum.blob.core.windows.net/reactspectrum/2c45388fdb4783f47e3f0d47112f89c90aafdc19/storybook/index.html?path=/story/react-aria-components-listbox--virtualized-list-box-grid&providerSwitcher-express=false

We added a 30px border to the virtualizer div aria-label="virtualized listbox", then tried drag and drop. On main, the drop indicator shows up on the next line down when you drag. We thought that this was what the PR was addressing, however, we saw the same behaviour here as well.

Can you clarify what this PR is addressing. What was broken and what it's fixing, as well as how to test it. Possibly adding a story could help.

Thanks!!

@nwidynski
Copy link
Contributor Author

nwidynski commented Sep 3, 2025

@snowystinger I'm confused, this change is in DOMLayoutDelegate, so why test virtualized scenarios? Am i missing the connection somewhere?

Otherwise, to give you more context on this change, it circles back to the pending RFC, which unfortunately is taking way longer to write up than I expected. I won't go much further into detail here, since it will become clear once that's PR'd, but essentially I'm using this delegate to scroll a "random access" item target rect to its scroll snap alignment - when the border is included, it causes target offsets not to lign up with visual position.

PS: Also noticed lately that contentSize should be diffed from first and last item instead of simply taking scrollWidth, in case scroll container is larger than content.

@snowystinger
Copy link
Member

Apologies, it was a lot of delegates to follow around.
This story works for testing https://reactspectrum.blob.core.windows.net/reactspectrum/2c45388fdb4783f47e3f0d47112f89c90aafdc19/storybook/index.html?path=/story/react-aria-components-listbox--async-list-box&providerSwitcher-express=false

Manually adding border: 100px solid red on our main branch currently "overshoots" if you are focused on the first item, then you press page down
In this branch, it goes to the same place regardless of the border

@LFDanLu LFDanLu added this pull request to the merge queue Sep 5, 2025
Merged via the queue into adobe:main with commit d847036 Sep 5, 2025
34 of 36 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants